Skip to content

Add server/config package.#320

Merged
Chounoki merged 8 commits into
openconfig:mainfrom
Chounoki:pr3
May 22, 2026
Merged

Add server/config package.#320
Chounoki merged 8 commits into
openconfig:mainfrom
Chounoki:pr3

Conversation

@Chounoki
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new configuration schema for the Bootz server, including Protobuf definitions, Bazel build rules, and generated Go code. The review feedback focuses on improving the Protobuf design and ensuring consistent Go package naming. Specifically, it is recommended to use the bytes type instead of Base64-encoded strings for binary data like certificates and keys to be more idiomatic and efficient. Additionally, the reviewer suggested adding option go_package to the proto file and updating the Bazel importpath to ensure the generated Go package name correctly reflects the intended structure.

Comment thread server/proto/BUILD.bazel Outdated
Comment thread server/proto/config/config.pb.go Outdated
Comment thread server/proto/config.proto
Comment thread server/proto/config.proto Outdated
Comment thread server/proto/config.proto
Comment thread server/proto/config.proto
@Chounoki Chounoki force-pushed the pr3 branch 2 times, most recently from f673437 to 7ef981f Compare May 18, 2026 10:07
@Chounoki Chounoki requested a review from gmacf May 18, 2026 10:16
@gmacf
Copy link
Copy Markdown
Contributor

gmacf commented May 19, 2026

Is this for Monax? If so, I would like to see how this fits in with the existing protos or proposed workflow. Do you have any example configs for this?

@Chounoki
Copy link
Copy Markdown
Contributor Author

Is this for Monax? If so, I would like to see how this fits in with the existing protos or proposed workflow. Do you have any example configs for this?

Yes, this is for Monax, so the vendors can use all real data like ownership_voucher.
I haven't created the PR yet, but when creating the Bootz server, this Config proto should be read from a file and provided to the Bootz server.

Comment thread server/proto/config.proto
Comment thread server/proto/config.proto Outdated
Comment thread server/proto/config.proto
@gmacf
Copy link
Copy Markdown
Contributor

gmacf commented May 19, 2026

Is this for Monax? If so, I would like to see how this fits in with the existing protos or proposed workflow. Do you have any example configs for this?

Yes, this is for Monax, so the vendors can use all real data like ownership_voucher. I haven't created the PR yet, but when creating the Bootz server, this Config proto should be read from a file and provided to the Bootz server.

In that case, could you either edit the existing Monax protos under server/tests/proto or move the config there? The reason I ask is because this file is very similar to server/tests/proto/test.proto

@Chounoki
Copy link
Copy Markdown
Contributor Author

Chounoki commented May 19, 2026

Is this for Monax? If so, I would like to see how this fits in with the existing protos or proposed workflow. Do you have any example configs for this?

Yes, this is for Monax, so the vendors can use all real data like ownership_voucher. I haven't created the PR yet, but when creating the Bootz server, this Config proto should be read from a file and provided to the Bootz server.

In that case, could you either edit the existing Monax protos under server/tests/proto or move the config there? The reason I ask is because this file is very similar to server/tests/proto/test.proto

Besides Monax, this proto definition will be referenced by artifact_manager and chassis_manager as argument in their respective New() function, as well as by server_emulator and client_emulator, so I am not sure whether putting it into the test directory is appropriate.

Even without doing any test (Monax or not), this proto definition is still required.

@gmacf
Copy link
Copy Markdown
Contributor

gmacf commented May 20, 2026

Is this for Monax? If so, I would like to see how this fits in with the existing protos or proposed workflow. Do you have any example configs for this?

Yes, this is for Monax, so the vendors can use all real data like ownership_voucher. I haven't created the PR yet, but when creating the Bootz server, this Config proto should be read from a file and provided to the Bootz server.

In that case, could you either edit the existing Monax protos under server/tests/proto or move the config there? The reason I ask is because this file is very similar to server/tests/proto/test.proto

Besides Monax, this proto definition will be referenced by artifact_manager and chassis_manager as argument in their respective New() function, as well as by server_emulator and client_emulator, so I am not sure whether putting it into the test directory is appropriate.

Even without doing any test (Monax or not), this proto definition is still required.

I'm not sure how this config proto will be used in the production (non-test instance). Doesn't this config have to contain an exhaustive list of the inventory and other artifacts? In the real world, we fetch these from external systems.

@Chounoki
Copy link
Copy Markdown
Contributor Author

Chounoki commented May 21, 2026

Is this for Monax? If so, I would like to see how this fits in with the existing protos or proposed workflow. Do you have any example configs for this?

Yes, this is for Monax, so the vendors can use all real data like ownership_voucher. I haven't created the PR yet, but when creating the Bootz server, this Config proto should be read from a file and provided to the Bootz server.

In that case, could you either edit the existing Monax protos under server/tests/proto or move the config there? The reason I ask is because this file is very similar to server/tests/proto/test.proto

Besides Monax, this proto definition will be referenced by artifact_manager and chassis_manager as argument in their respective New() function, as well as by server_emulator and client_emulator, so I am not sure whether putting it into the test directory is appropriate.
Even without doing any test (Monax or not), this proto definition is still required.

I'm not sure how this config proto will be used in the production (non-test instance). Doesn't this config have to contain an exhaustive list of the inventory and other artifacts? In the real world, we fetch these from external systems.

This config profo is only for the open source Bootz. This config proto itself as well as its consumers: artifact_manager, chassis_manager, bootz_emulator and client_emulator won't be sync'ed back to our internal codebase.

Our internal Bootz will use a different version of artifact_manager and chassis_manager, which implements the same interfaces as the open source ones.

@Chounoki
Copy link
Copy Markdown
Contributor Author

Chounoki commented May 21, 2026

Is this for Monax? If so, I would like to see how this fits in with the existing protos or proposed workflow. Do you have any example configs for this?

Yes, this is for Monax, so the vendors can use all real data like ownership_voucher. I haven't created the PR yet, but when creating the Bootz server, this Config proto should be read from a file and provided to the Bootz server.

In that case, could you either edit the existing Monax protos under server/tests/proto or move the config there? The reason I ask is because this file is very similar to server/tests/proto/test.proto

Besides Monax, this proto definition will be referenced by artifact_manager and chassis_manager as argument in their respective New() function, as well as by server_emulator and client_emulator, so I am not sure whether putting it into the test directory is appropriate.
Even without doing any test (Monax or not), this proto definition is still required.

I'm not sure how this config proto will be used in the production (non-test instance). Doesn't this config have to contain an exhaustive list of the inventory and other artifacts? In the real world, we fetch these from external systems.

This config profo is only for the open source Bootz. This config proto itself as well as its consumers: artifact_manager, chassis_manager, bootz_emulator and client_emulator won't be sync'ed back to our internal codebase.

Our internal Bootz will use a different version of artifact_manager and chassis_manager that implement the same interface.

I have created another PR of artifact_manager to demonstrate how this config proto is consumed, but due to the dependency, I have to create the new PR containing both artifact_manager package and this config proto package together in the same PR, otherwise the new PR will fail the building test due to dependency.

@Chounoki
Copy link
Copy Markdown
Contributor Author

Chounoki commented May 21, 2026

I think we can put Monax aside for now, because we are still too far away from implementing any Monax test.

The major use of this config proto definition is to replace the current Bootz server emulator command line arguments defined as below:
https://github.com/openconfig/bootz/blob/main/server/emulator/main.go#L38-L42

I will remove all those command line arguments and only use a single textproto file generated from this config proto definition as the command line argument for starting a Bootz server emulator.

The client emulator will do the same, by also using the same single textproto file generated from this config proto definition as the command line argument for starting a Bootz client emulator.

How we should start the Monax Bootz test can be discussed later and doesn't have to use this config proto, but we still need this proto for the server+client emulator. Currently those emulators generate lots of fake certs/keys/vouchers internally, but we can't do that anymore as we are going to ask the vendors to test with real switches, so all those things will be provided by this textproto.

I hope my explanation makes things a little clearer now.

@Chounoki Chounoki merged commit 5f62e61 into openconfig:main May 22, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants